Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Platform encapsulation #235

Merged
merged 14 commits into from
Nov 29, 2021
Merged

Conversation

ben-xD
Copy link
Contributor

@ben-xD ben-xD commented Nov 18, 2021

@ikbalkaya started a conversation about the generous use of statics in platform.dart. #219 (comment)

Sorry to be picky on static methods but are we not performing this methods on Platform object here? Using statics will defeat the purpose of encapsulation and will cause the design to consume other static states which makes application unmaintainable. I think those static methods, fields should be used a bit more cautiously

This PR makes Platform a singleton instead of having every method and field a static one, and removes direct usages of Platform.methodChannel and Platform.streamsChannel outside of Platform class (encapsulation).

It also removes any complexity which was added to handle hot restart issues. This was entirely due to using static methods. Now that we have a constructor in Platform.dart, hot restarts works for free: the constructor is called at launch and whenever hot restart occurs. I owe this discovery to @ikbalkaya, thanks 👍 . Keep asking good (and difficult) questions 😃

Also includes small log line improvements which I noticed could be improved

Note about testing

I added optional arguments to the. Platform singleton for tests to override the method channel. This allows them to configure method channel (call setMockMethodCallHandler) without manually accessing the internal state of Platform after it is instantiated.

Quintin's concern

I remember reload and restart being pretty tricky to debug and test when we were in the early days of developing this plugin

Hot reload and restart are not tricky in themselves. The complexity in the code arose from a misunderstanding of Flutter, and a misuse of static.

  • When hot reload happens, no code is executed, so we don't have to worry. We don't need to do anything.
  • When hot restart happens, we want to reset the client instances the Dart side loses references (handles) to the ably clients on the platform side. There was a lot of logic to check if a hot restart happened, and was only necessary when using static methods. If we have a singleton + constructor, this is called everytime the app hot restarts or restarts. We can just call the platform method to reset the Ably instances. I called this resetAblyClients.

@github-actions github-actions bot temporarily deployed to staging/pull/235/dartdoc November 18, 2021 16:13 Inactive
…and make `methodChannel` internal to `platform.dart`
@ben-xD ben-xD force-pushed the enhancement/platform-encapsulation branch from d7f649b to 74a1dad Compare November 18, 2021 18:41
@ben-xD ben-xD marked this pull request as ready for review November 18, 2021 18:42
@github-actions github-actions bot temporarily deployed to staging/pull/235/dartdoc November 18, 2021 18:44 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/235/dartdoc November 18, 2021 18:52 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/235/dartdoc November 18, 2021 23:04 Inactive
@ben-xD ben-xD changed the base branch from enhancement/run-unit-tests-in-IDE to enhancement/rename-pushSetOnBackgroundMessage-to-pushBackgroundFlutterApplicationReadyOnAndroid November 18, 2021 23:06
…ckgroundFlutterApplicationReadyOnAndroid' into enhancement/platform-encapsulation

# Conflicts:
#	android/src/main/java/io/ably/flutter/plugin/BackgroundMethodCallHandler.java
#	lib/src/platform/src/push_notification_events_native.dart
@github-actions github-actions bot temporarily deployed to staging/pull/235/dartdoc November 18, 2021 23:10 Inactive
…ckgroundFlutterApplicationReadyOnAndroid' into enhancement/platform-encapsulation

# Conflicts:
#	lib/src/platform/src/push_notification_events_native.dart
@github-actions github-actions bot temporarily deployed to staging/pull/235/dartdoc November 18, 2021 23:12 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/235/dartdoc November 18, 2021 23:20 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/235/dartdoc November 18, 2021 23:38 Inactive
…ckgroundFlutterApplicationReadyOnAndroid' into enhancement/platform-encapsulation

# Conflicts:
#	lib/src/platform/src/platform.dart
@github-actions github-actions bot temporarily deployed to staging/pull/235/dartdoc November 23, 2021 08:35 Inactive
Copy link
Contributor

@ikbalkaya ikbalkaya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me 😎 Thanks for addressing this

Base automatically changed from enhancement/rename-pushSetOnBackgroundMessage-to-pushBackgroundFlutterApplicationReadyOnAndroid to main November 26, 2021 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants